Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

hyphen-case config key support #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frohoff
Copy link

@frohoff frohoff commented May 6, 2015

This is a draft PR to implement #7 using Guava's CaseFormat class for the camelCase to hyphen-case conversion.

Created separate HyphenCaseArbitraryTypeReader trait and object (existing ArbitraryTypeReader API/behavior is unchanged) that invokes a new macro that specifies a hyphen-case NameMapper trait implementation that gets passed down to the extractMethodArgsFromConfig() method. Added a test case to ArbitraryTypeReaderSpec.

Probably could be reorganized a bit. Please advise.

@ceedubs
Copy link
Owner

ceedubs commented May 6, 2015

Thanks, @frohoff! This is a useful addition. I really like the idea of passing a name transformation into extractMethodArgsFromConfig, and I think we should go forward with this.

Here are a couple thoughts, though.

Currently Ficus's only runtime dependency is Typesafe config, and I'd prefer to keep it that way. I've gotten into a dependency nightmare with Guava version mismatches before, and they are no fun. CaseFormat is indeed handy, but I'm not sure that we should be deciding whether people want to use hyphens, underscores, etc.

Separately, it doesn't seem to me that the NameMapper trait is providing any more benefit than a simple nameMapper: String => String could provide.

Therefore, I propose that we do the following:

  • Get rid of NameMapper, and instead use a String => String argument (which can be identity for the existing code)
  • Get rid of the Guava dependency and let users pass in their own function, which may be CaseFormat.LOWER_CAMEL.to(CaseFormat.LOWER_HYPHEN, _) or whatever other transformation they care to make. They can then create their own HyphenCaseArbitraryTypeReader (or equivalent) trait/object.

What do you think?

@frohoff
Copy link
Author

frohoff commented May 6, 2015

I had actually originally intended to implement with a user-specifiable mapping function it similar to how you suggested but it was seeming quite complicated to pass parameters through the macro definition into the implementation and helper methods, though my experience with macros is pretty limited and it's possible there's something I'm not considering. With this in mind, I was hesitant to do anything too convoluted in the implementation, and without a mechanism to pass parameters through, it would probably leave ficus library users to write their own macro definitions which seemed like something one would want to avoid.

I'm definitely open to suggestions/direction.

@ceedubs
Copy link
Owner

ceedubs commented May 7, 2015

Ah, thanks for the explanation. I didn't realize it was so difficult to pass custom arguments to macros. I'm wondering if we could still use String => String without NameMapper though it looks like we will still have to do it via macro like you have said.

I'm still pretty hesitant to introduce the Guava dependency. Maybe it would make sense to have a separate module that could add the Guava dependency and have helpers for a few different formats? I don't know, I'll ponder this a bit.

@frohoff
Copy link
Author

frohoff commented May 7, 2015

Agreed on dropping NameMapper in favor of String => String. In the meantime I'll see if I can get something working using annotations. Also, we can consider making Guava an optional (compile-time only) dependency that people need to include on their own and just ensure that all load-time dependencies to it are in a separate class file so that it doesn't throw a NoClassDefFoundError when using the existing ArbiraryTypeReader code without Guava on the classpath.

@ceedubs
Copy link
Owner

ceedubs commented May 7, 2015

I would say don't sink too much time using the annotation approach for now. It's complicated and probably less flexible than other approaches. I have some ideas I would like to try out, but it might be a few days before I get to it.

@frohoff
Copy link
Author

frohoff commented May 7, 2015

One nice thing about the annotation approach is that it might enable further customization of these readers for things like the exhaustive key-parameter mapping I described in #13.

@frohoff
Copy link
Author

frohoff commented May 13, 2015

I threw together a rough working prototype that supports customizable parameter mapping and exhaustivity checking (for #13) using annotations at frohoff/ficus@54cb7d1 but then had another idea:

Another way to do this that seemed a bit cleaner was to have the macro compile in calls/references to members on the trait/object it's being called "through" (the macro's c.prefix) that could be overridden for customization by users:

A rough working prototype for this is at frohoff/ficus@57de54d

This ends up looking roughly like this:

// user code
object CustomizedArbitraryTypeReader extends ArbitraryTypeReader {
  override val checkExhaustivity: Boolean = true
  override def mapParams(n: String) = CaseFormat.LOWER_CAMEL.to(CaseFormat.LOWER_HYPHEN, n)
}

// library code
trait ArbitraryTypeReader {
  implicit def arbitraryTypeValueReader[T]: ValueReader[T] = macro ArbitraryTypeReaderMacros.arbitraryTypeValueReader[T]
  val checkExhaustivity: Boolean = false
  def mapParams(n: String) = n
}

This should also allow implementing common options as modular traits a la:

// user code
object MyArbitraryTypeReader 
  extends ArbitraryTypeReader 
  with GuavaHyphenParamMapping // optional guava dependency or in separate module
  with ExhaustivityChecking

@shanielh
Copy link

Looking forward for this to be merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants